Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add linux aarch64 wheel support #183

Closed
wants to merge 1 commit into from
Closed

Conversation

odidev
Copy link

@odidev odidev commented Mar 24, 2021

Added linux aarch64 wheel support.
Related to #182. @ajfriend Could you please review this PR?

@CLAassistant
Copy link

CLAassistant commented Mar 24, 2021

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Mar 24, 2021

Codecov Report

Merging #183 (ec27965) into master (c3593ec) will not change coverage.
The diff coverage is n/a.

❗ Current head ec27965 differs from pull request most recent head 01fb493. Consider uploading reports for the commit 01fb493 to get more accurate results
Impacted file tree graph

@@            Coverage Diff            @@
##            master      #183   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            9         9           
  Lines          213       213           
=========================================
  Hits           213       213           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c3593ec...01fb493. Read the comment docs.

@ajfriend
Copy link
Contributor

This is great! Thank you!

My first few thoughts:

  • Would it be possible to add tests to tests.yml? I see that the tests run during the wheels.yml, but it would also be nice to have a set of tests that apply to aarch64 but don't require building wheels.
    • I suppose QEMU is needed for cibuildwheel to do its job? Maybe the setup is similar for running non-wheel tests?
  • Any reason we can't build cp35-*? It looks like cibuildwheel supports it.
  • Any reason we need the arm64_make_wheels job and can't use the make_wheels job? It would be nice, if possible, to reuse the ## Setup Env and ## Upload steps, and just add to the ## Build across platforms step. Unless you've spotted a reason why we can't do that, I could potentially help with figuring that out.
  • Motivated mostly by my own laziness and ignorance, do you have any idea how easily this would extend to "macOS Apple Silicon" for cp39-*? If non-trivial, we can make a ticket for it; I'm sure that request is imminent :)

@odidev
Copy link
Author

odidev commented Mar 25, 2021

Please find quoted replies:

  • Would it be possible to add tests to tests.yml? I see that the tests run during the wheels.yml, but it would also be nice to have a set of tests that apply to aarch64 but don't require building wheels.

Done.

  • I suppose QEMU is needed for cibuildwheel to do its job? Maybe the setup is similar for running non-wheel tests?

Yes, for aarch64 qemu is required for emulation.

Python3.5 is now depreciated that is why I removed it.

  • Any reason we need the arm64_make_wheels job and can't use the make_wheels job? It would be nice, if possible, to reuse the ## Setup Env and ## Upload steps, and just add to the ## Build across platforms step. Unless you've spotted a reason why we can't do that, I could potentially help with figuring that out.

In emulation environment the aarch64 build is slower. To make it faster I have split the job as per python version.
If we use the same job it will become slower.

  • Motivated mostly by my own laziness and ignorance, do you have any idea how easily this would extend to "macOS Apple Silicon" for cp39-*? If non-trivial, we can make a ticket for it; I'm sure that request is imminent :)

I will check and update you regarding this.

@ajfriend
Copy link
Contributor

Thanks! This looks basically ready to merge.

Just to check before merging, have you considered https://github.com/uraimo/run-on-arch-action ? I wonder if it could help simplify, for example, the code in tests.yml.

I don't think its a blocker. I'm just trying to better understand the options for the sake of future maintenance. 😄

@ajfriend
Copy link
Contributor

ajfriend commented Apr 1, 2021

Just for future reference, I want to note these issues, which seem to have a lot of relevant information and link to a few projects that have also done this with cibuildwheel:

@odidev
Copy link
Author

odidev commented Apr 21, 2021

@ajfriend, could you please review this PR? Thanks.

@ajfriend
Copy link
Contributor

I should be able to take a look at it this week. I definitely appreciate the effort and help on this, @odidev!

@odidev
Copy link
Author

odidev commented May 5, 2021

@ajfriend, I could see the changes are approved. Could you please let me know when this will get merged? Thanks in advance.

@ajfriend
Copy link
Contributor

ajfriend commented Jun 8, 2021

Hey @odidev, sorry for the delay.

  • Could we actually remove the changes to tests.yml for now? Thank you for putting that together, but that's more code than I'd want to add just to add the pre-wheel tests. I'd love to add such tests if we could find a way with a smaller footprint.
  • Could you update in light of ci: cleanup and pin cibuildwheel #188?
  • And what would the diff look like if you did it through the existing make_wheels job? I know you mentioned it is slower, but if it is much simpler than having an additional arm64_make_wheels, I might prefer that. What's the time difference between the two options?

@odidev
Copy link
Author

odidev commented Jun 8, 2021

Hey @odidev, sorry for the delay.

  • Could we actually remove the changes to tests.yml for now? Thank you for putting that together, but that's more code than I'd want to add just to add the pre-wheel tests. I'd love to add such tests if we could find a way with a smaller footprint.

Sure I can remove test.yml changes.

The changes can be done as #188.

  • And what would the diff look like if you did it through the existing make_wheels job? I know you mentioned it is slower, but if it is much simpler than having an additional arm64_make_wheels, I might prefer that. What's the time difference between the two options?

I can surely add aarch64 jobs in make_wheels job, the time to build aarch64 wheels is around 1 hr. On the other hand, by adding arm64_make_wheels jobs and dividing aarch64 jobs for each python version, each job takes around 11 min.
However for reducing time and to go ahead with one job for both architectures (auto and aarch64), the jobs can be divided as per python version.

@ajfriend, Please let me know your views on this and I will do the changes accordingly.

@ajfriend
Copy link
Contributor

ajfriend commented Jun 8, 2021

I can surely add aarch64 jobs in make_wheels job, the time to build aarch64 wheels is around 1 hr. On the other hand, by adding arm64_make_wheels jobs and dividing aarch64 jobs for each python version, each job takes around 11 min.
However for reducing time and to go ahead with one job for both architectures (auto and aarch64), the jobs can be divided as per python version.

Ah, I see. You're splitting up the jobs by Python version. I didn't catch that that was the trick before. That's a nice idea!

It looks like #189 is working with the ~1 hour build time. Let's land #189 that for now, and potentially follow up--either in this PR or another--with something to split up the hour by Python version. I can definitely imagine that hour turnaround being annoying next time we do a release. :)

@ajfriend ajfriend mentioned this pull request Jun 8, 2021
@ajfriend
Copy link
Contributor

ajfriend commented Jun 9, 2021

It looks like we have things working in #189. Thank you for kicking off this workstream, @odidev!

After we look into #191 I'll make a release to publish the aarch64 wheels to PyPI. @odidev, could you let us know if those work for you?

Closing this. Feel free to re-open if there are any dangling issues I've fogotten.

@ajfriend ajfriend closed this Jun 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants